fix: Catch local API server various errors #6548
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Vanalite
changed the title
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to ecae0f5 in 2 minutes and 0 seconds. Click for details.
- Reviewed
84lines of code in2files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src-tauri/src/core/server/proxy.rs:752
- Draft comment:
Good use of Server::try_bind to catch binding errors. Consider adding contextual information (e.g. with anyhow's context) when returning the error for easier debugging. - Reason this comment was not posted:
Confidence changes required:30%<= threshold50%None
2. web-app/src/routes/settings/local-api-server.tsx:187
- Draft comment:
In the catch block, consider using 'instanceof Error' for type-safe extraction of the error message instead of checking 'message' in error. This improves robustness when dealing with unknown error types. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The current code is actually more flexible than usinginstanceof Errorsince it can handle any object with a message property, not just Error instances. In a TypeScript/JavaScript environment, especially when dealing with errors from various sources (server, model loading, etc.), errors may not always be proper Error instances. The current approach is more defensive and handles a wider range of error formats. The suggestion isn't necessarily wrong - using instanceof Error would be more type-safe. However, it might be too restrictive and could miss valid error messages from non-Error objects. The current implementation is actually a better pattern for this specific use case where errors could come from multiple sources and in various formats. The code needs to be flexible rather than strictly type-safe. The comment should be deleted. The current error handling approach is more appropriate for this context where flexibility is more important than strict type safety.
3. web-app/src/routes/settings/local-api-server.tsx:236
- Draft comment:
The getButtonText function refers to 'isServerRunning' which is declared later (line 256). Although function execution order ensures it's defined by render time, reordering these declarations for clarity would improve code readability. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_pGggCIttHstzIwcD
You can customize 
Barecheck - Code coverage reportTotal: 30.53%Your code coverage diff: -0.03% ▾ Uncovered files and lines
|
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 68d318a in 2 minutes and 45 seconds. Click for details.
- Reviewed
179lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/hooks/__tests__/useLocalApiServer.test.ts:447
- Draft comment:
No test covers the 'model file path invalid' error mentioned in the PR description; please add a case for it. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment references something from the PR description which I don't have access to. Without seeing the PR description or the actual implementation code, I can't verify if this is a real issue or if testing this scenario is even relevant. The comment is speculative since it's based on information outside the diff. Additionally, asking for test coverage based solely on PR description text violates the rule about not making assumptions about PR descriptions. Maybe there really is an important error case that needs testing. Without seeing the PR description or implementation, I could be wrong about dismissing this. Even if there is a real error case to test, comments should be self-contained and based on the code changes shown, not external documentation. The reviewer should point to specific code that needs testing. Delete this comment as it relies on external context from the PR description and violates the rule about not making assumptions about PR descriptions.
2. web-app/src/hooks/__tests__/useLocalApiServer.test.ts:536
- Draft comment:
Missing a regression test for issue #6546 (failure to start the server after a previously selected imported model is removed). - Reason this comment was not posted:
Comment was on unchanged code.
3. web-app/src/hooks/__tests__/useLocalApiServer.test.ts:591
- Draft comment:
Add a newline at the end of the file to comply with standard file formatting conventions. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_qbOEYUuhI8vOSFRM
You can customize
Vanalite
force-pushed
the
fix/local_api_server_error_catch
branch
from
68d318a to
2eef4af
Compare
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 2eef4af in 1 minute and 48 seconds. Click for details.
- Reviewed
254lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. web-app/src/hooks/__tests__/useLocalApiServer.test.ts:431
- Draft comment:
The PR description mentions catching errors for an invalid model file path, but no test covers that scenario. Consider adding a test case to simulate and verify the handling of an invalid model file path. - Reason this comment was not posted:
Comment was on unchanged code.
2. web-app/src/hooks/__tests__/useLocalApiServer.test.ts:666
- Draft comment:
Ensure the file ends with a newline to conform with best practices. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. web-app/src/hooks/__tests__/useLocalApiServer.test.ts:467
- Draft comment:
The use of toString() on apiKey is redundant since apiKey is already a string. Consider removing the toString() call. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_StgCRCUx6f00u7Lh
You can customize
|
I am trying to test the case 
This is because:
|
|
LGTM! 
|
Minh141120
approved these changes
Sep 23, 2025
dinhlongviolin1
added a commit
that referenced
this pull request
Sep 23, 2025
* ✨ feat: Re-arrange docs as needed * 🔧 chore: re-arrange the folder structure * Add server docs Add server docs * enhancement: migrate handbook and janv2 * Update docs/src/components/ui/dropdown-button.tsx Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com> * Update docs/src/pages/_meta.json Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com> * chore: update feedback #1 * fix: layout ability model * feat: add azure as first class provider (#6555) * feat: add azure as first class provider * fix: deployment url * Update handbook: restructure content and add new sections - Add betting-on-open-source.mdx and open-superintelligence.mdx - Update handbook index with new structure - Remove outdated handbook sections (growth, happy, history, money, talent, teams, users, why) - Update handbook _meta.json to reflect new structure * chore: fix meta data json * chore: update missing install * fix: Catch local API server various errors (#6548) * fix: Catch local API server various errors * chore: Add tests to cover error catches * fix: LocalAPI server trusted host should accept asterisk (#6551) * feat: support .zip archives for manual backend install (#6534) * feat(llamacpp): support .zip archives for manual backend install * Update Lock Files * Merge pull request #6563 from menloresearch/feat/web-minor-ui-tweak-login feat: tweak login UI --------- Co-authored-by: LazyYuuki <[email protected]> Co-authored-by: nngostuds <[email protected]> Co-authored-by: Faisal Amir <[email protected]> Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com> Co-authored-by: Louis <[email protected]> Co-authored-by: eckartal <[email protected]> Co-authored-by: Nghia Doan <[email protected]> Co-authored-by: Roushan Kumar Singh <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe Your Changes
Add various error catching during the start of Local API server
Fixes Issues
Test plan
Port occupation error catch
Model path error catch
Self Checklist
Important
Improves error handling for Local API server by catching port conflicts and model path errors, with added tests and UI feedback.
proxy.rs, added error handling for port binding failures instart_server().local-api-server.tsx, added error handling for port conflicts and invalid model paths intoggleAPIServer().useLocalApiServer.test.ts, added tests for port conflict error messages and API key validation.local-api-server.tsx, added toast notifications for specific errors like port occupation and model path issues.This description was created by
for 2eef4af. You can customize this summary. It will automatically update as commits are pushed.